Removing unused code, take 2. #1648
Conversation
I want to get this commit into the history because the tests pass here, which demonstrates that every commented out method is not only unnecessary internally but has zero test coverage. Since I know (based on the occasional source code comment, or more often based on knowing something about other source bases) that some of these can't be removed without breaking other things, I want to at least record a snapshot of the identities of all these unused and untested methods. This commit will be reverted; then there will be another commit which removes the subset of these methods which I believe to be removable. The remainder are in great need of tests which exercise the interfaces upon which other repositories depend.
This reverts commit 951fc3a.
Translating <code></code> into backticks. Removed the "@param tree ..." blocks which have been taunting me for half a decade now. Removed commented-out blocks of code which had been sitting there for two years or more.
And small associated changes.
There were a whole lot of these.
Mostly hailing from a long-ago day when I imagined I was writing a general purpose library. We dodged that bullet.
It has accreted its share through the bumpy years.
There was a problem hiding this comment.
Is this intentional or just an oversight?
There was a problem hiding this comment.
"Is that a donkey or a baseball?"
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1691/ |
|
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1691/ |
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/981/ |
|
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/981/ |
|
Paul, two Sbt modules that we bundle in the IDE for incremental compilation do not compile anymore. Sorry for the trouble, yesterday I must have messed up something when locally publishing the Scala artifacts. Here is the error (it is the same for both Sbt modules: compile and compiler-interface):
Then, while scala-refactoring now compiles fine, there are a number of failing tests (which runs just fine with the last deployed 2.11.0-SNAPSHOT). Maybe we should ask for Mirko (@misto) feedback.
Finally, there is a new error in the IDE side:
|
|
@dotta can you send me the detailed error reports of the failures? |
|
Sure, here you go http://dl.dropbox.com/u/50945416/scala-refactoring-surefire-reports.zip |
|
Ok, it seems all the failures have the same cause: "Race condition detected: You are running a presentation compiler method outside the PC thread." Should be easy to fix! |
|
Oh, I see, thanks Mirko for looking into it! And, I just noticed that our nightly for scala-refactoring against 2.11.0-SNAPSHOT is not running tests. That explains why I did not see the failing tests before. Bottom line, I believe we can ignore the failing tests in scala-refactoring, as they are not related to this PR. If we can sort out the two remaining compilation errors, you'll have my 👍 ;-) |
Not a bad showing for a newcomer. Of course most of this code predates scala.reflect by a lot.
Removing code from this neighborhood is more difficult than elsewhere, making it all the more important that it be done.
Nobody is immune!
They are everywhere.
They defy categorization.
They are... M I S C
All those old-timey methods whose melodies have become unfashionable.
|
If I understood which errors were legit and which weren't, the push I just did should get everything compiling. |
|
PLS REBUILD ALL |
|
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1695/ |
|
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1695/ |
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/985/ |
|
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/985/ |
Yes, now it's all good from the IDE side. |
|
sure sounds like an LGTM to me! excellent work, gang! |
Removing unused code, take 2.
|
so long, lines of code! |
|
Surely as a reward @paulp gets the honour of driving the stake through patmat classico on master. |
|
that, and a complimentary cappuccino |
Follow-up on sbt#314 - I _still_ misinterpreted.. Turns out the ".asInstanceOf[AnyRef].getClass.getName" implementation was the _original_ implementation. Then Mark switched to using bindValue in sbt/sbt@4b8f0f3. Since Scala 2.11.0 (scala/scala#1648 in particular) bindValue was removed. So we'll use NamedParam and quietBind, both which exist since Scala 2.9.0. Fixes sbt/sbt#2884, tested with local releases.
Follow-up on sbt#314 - I _still_ misinterpreted.. Turns out the ".asInstanceOf[AnyRef].getClass.getName" implementation was the _original_ implementation. Then Mark switched to using bindValue in sbt/sbt@4b8f0f3. Since Scala 2.11.0 (scala/scala#1648 in particular) bindValue was removed. So we'll use NamedParam and quietBind, both which exist since Scala 2.9.0. Fixes sbt/sbt#2884, tested with local releases.
Follow-up on sbt#314 - I _still_ misinterpreted.. Turns out the ".asInstanceOf[AnyRef].getClass.getName" implementation was the _original_ implementation. Then Mark switched to using bindValue in sbt/sbt@4b8f0f3. Since Scala 2.11.0 (scala/scala#1648 in particular) bindValue was removed. So we'll use NamedParam and quietBind, both which exist since Scala 2.9.0. Fixes sbt/sbt#2884, tested with local releases.
Follow-up on scala#314 - I _still_ misinterpreted.. Turns out the ".asInstanceOf[AnyRef].getClass.getName" implementation was the _original_ implementation. Then Mark switched to using bindValue in sbt/sbt@4b8f0f3. Since Scala 2.11.0 (scala#1648 in particular) bindValue was removed. So we'll use NamedParam and quietBind, both which exist since Scala 2.9.0. Fixes sbt/sbt#2884, tested with local releases.
Follow-up on scala#314 - I _still_ misinterpreted.. Turns out the ".asInstanceOf[AnyRef].getClass.getName" implementation was the _original_ implementation. Then Mark switched to using bindValue in sbt/sbt@4b8f0f3. Since Scala 2.11.0 (scala#1648 in particular) bindValue was removed. So we'll use NamedParam and quietBind, both which exist since Scala 2.9.0. Fixes sbt/sbt#2884, tested with local releases. Rewritten from sbt/zinc@33d2e68
7 participants
Incorporating knowledge of what failed on the first attempt. The list of failures probably looked like a lot more than it was; only five methods were implicated, but some were called a lot. This PR restores those methods and marks them as externally used until we have a more permanent mechanism for assuring their survival.
Review by @dotta (confirmation that everything builds against this would be very helpful.)